feat!: Remove spanId from propagation context
#14733
Conversation
size-limit report 📦
|
| @@ -1,230 +0,0 @@ | |||
| import { Scope, getGlobalScope, prepareEvent } from '@sentry/core'; | |||
There was a problem hiding this comment.
This test is not needed, it is just a duplicate of the core tests. this is a leftover from the experimental days when node had a forked/custom scope, but it is the same now.
| if (isSpanContextValid(parentContext) && parentContext.traceId === traceId) { | ||
| if (parentContext.isRemote) { | ||
| const parentSampled = getParentRemoteSampled(parentSpan); | ||
| const parentSampled = getSamplingDecision(parentSpan.spanContext()); |
There was a problem hiding this comment.
This method implementation was redundant, we already check traceId outside here, so we can simply get the decision directly. This was the only place where we still used getPropagationContextFromSpan.
mydea
force-pushed
the
fn/removeSpanId
branch
2 times, most recently
from
5a72bc7 to
4eb2604
Compare
mydea
force-pushed
the
fn/removeSpanId
branch
from
00ad048 to
8d7f03f
Compare
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I think this is in a good state, given we already made the decision that it's okay TwP scenarios to have different spanIds in consecutive errors or calls (see my other comment as well).
Before we merge this, I'd like to ensure that we have the following scenarios covered:
- [TwP] trace id within
traceenvelope header is equal toevent.contexts.trace - The
sentry-traceheader contains the id of thehttp.clientspan of the request
I'm fairly sure we already test this but it's probably worth to double check it
| expect(traceData['sentry-trace']).toMatch(/^[a-f0-9]{32}-[a-f0-9]{16}$/); | ||
| expect(traceData['sentry-trace']).toContain(`${trace_id}-`); | ||
| // span_id is a random span ID | ||
| expect(traceData['sentry-trace']).not.toContain(span_id); | ||
|
|
||
| expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`); | ||
| expect(traceData.baggage).not.toContain('sentry-sampled='); | ||
|
|
||
| expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-${span_id}"/>`); | ||
| expect(traceData.metaTags).toMatch(/<meta name="sentry-trace" content="[a-f0-9]{32}-[a-f0-9]{16}"\/>/); | ||
| expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-`); | ||
| // span_id is a random span ID | ||
| expect(traceData.metaTags).not.toContain(span_id); |
There was a problem hiding this comment.
hmm so I'm a bit concerned about this. IIUC previously, the spanIds in this test were equal because we stored it on the PC. Now, consecutive calls to APIs like getTraceData() or getTraceMetaTags() like in this test deliver diverging span ids.
This also implies the following scenario for a TwP-configured SDK:
- app makes http request and propagates a
sentry-traceheader with spanId123 - directly afterwards an error is captured and its
event.context.traceholds a different spanId456
I thought about the implications of this and while I'm still worried, I couldn't really come up with a concrete use case where this is actually problematic. I was especially worried if we'd send different spanIds within one error or transaction event but I think we only send this in event.context.trace. The trace envelope header does not contain the spanId at all, so it shouldn't be a problem.
However, we should ensure that the same traceId is still used in both places. Which I hope we have tests for but it's better to double-check.
There was a problem hiding this comment.
yes, so the traceId is tested here (see line 53).
I totally get what you mean, it was the same for me - thinking, hmm, this is different, but then, is it actually a problem?
Closes #12385 This also deprecates `getPropagationContextFromSpan` as it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there... fix tests remove unneeded test fix stuff fix tests better test fix test
Co-authored-by: Luca Forstner <[email protected]>
Closes #12385
This also deprecates
getPropagationContextFromSpanas it is no longer used/needed. We may think about removing this in v9, but IMHO we can also just leave this for v9, it does not hurt too much to have it in there...